Skip to content

rand: fix error check of RAND_load_file() - #1087

Merged
rhenium merged 1 commit into
ruby:masterfrom
ndossche:RAND_load_file-fix
Aug 5, 2026
Merged

rand: fix error check of RAND_load_file()#1087
rhenium merged 1 commit into
ruby:masterfrom
ndossche:RAND_load_file-fix

Conversation

@ndossche

@ndossche ndossche commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

This function returns -1 on error, or the number of bytes read on success. Preserve current behaviour and also error out on error. Discovered by an experimental static analyzer I work on.

Comment thread ext/openssl/ossl_rand.c Outdated
ossl_rand_load_file(VALUE self, VALUE filename)
{
if(!RAND_load_file(StringValueCStr(filename), -1)) {
if (RAND_load_file(StringValueCStr(filename), -1) <= 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. The current error check is clearly wrong: ruby -ropenssl -e'p OpenSSL::Random.load_random_file("not a file")' #=> true

Isn't a return value of 0 actually a success (indicating that the file was empty)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. I was mainly unsure whether you wanted to keep the behaviour for empty files the same as now. I think this can be argued about.
I'll drop the 0 check.

This function returns -1 on error, or the number of bytes read on
success. Preserve current behaviour and also error out on error.
Discovered by an experimental static analyzer I work on.
@ndossche
ndossche force-pushed the RAND_load_file-fix branch from d2c8dad to b36a615 Compare August 5, 2026 16:18
@rhenium

rhenium commented Aug 5, 2026

Copy link
Copy Markdown
Member

Test failure with OpenSSL master is irrelevant.

Thanks!

@rhenium
rhenium merged commit e324933 into ruby:master Aug 5, 2026
47 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants